Repository navigation
STF-1906: Add phone verification inputs to minFraud - #6
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
cf60e92 to
444a3ae
Compare
34f8da7 to
5f40f16
Compare
The minFraud request `billing` and `shipping` objects now accept three optional inputs that describe the most recent verification of the phone number: `phone_verification_method`, `phone_was_verification_successful`, and `phone_verification_time`. Risk scoring uses the shipping phone when the billing phone is absent. Both objects share the `Address` schema, so the inputs live there. The request example and the bundled spec include them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5f40f16 to
25b86c5
Compare
horgh
left a comment
There was a problem hiding this comment.
Some nits from Claude and a thought from me!
| - `other`: another verification method. | ||
| phone_verification_time: | ||
| type: string | ||
| format: date-time |
There was a problem hiding this comment.
format: date-time accepts some timestamps that the server rejects. JSON Schema follows RFC 3339, which allows a lowercase t and z and a leap second (:60). The server parses with time.Parse(time.RFC3339, ...), which rejects all of these:
2026-10-01t14:30:00z -> cannot parse "t14:30:00z" as "T"
2026-10-01T14:30:00z -> cannot parse "z" as "Z07:00"
2016-12-31T23:59:60Z -> second out of range
The server then returns INPUT_INVALID and drops the field. A client that validates against this spec does not see the problem until it reads the warning. The Python client PR for the same change uses an uppercase-only pattern, so the spec and that client disagree.
Suggestion: add a pattern that requires an uppercase T and Z, or say in the description that the server requires them and rejects leap seconds. The existing time input has the same gap, so a shared pattern for both fields can fix it in one place.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Done in f662b29. Both phone_verification_time and the event time now have a pattern that allows only an uppercase T and Z, seconds 00 to 59, and offsets up to 23:59.
Reply by Claude Opus 5.5 on Greg's behalf.
| - `delivered_code`: a code delivered to the phone, such as by SMS, | ||
| voice call, or messaging app. | ||
|
|
||
| - `network`: verification through the mobile network operator, such as |
There was a problem hiding this comment.
The network and other descriptions do not match the docs in dev-site#1775:
- Docs:
networkis "Carrier or SIM-based silent network authentication. No code is sent to the device." The spec says "verification through the mobile network operator", which can also describe flash-call or missed-call checks. - Docs:
otherincludes "flash or missed call verification, or a method you do not know because a third party performed the check". The spec says only "another verification method".
A merchant who uses flash-call verification and reads the spec can send network, but the docs say other. A merchant whose vendor does not report the method can omit the field, but the docs say to send other. Suggest using the docs wording for both values.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Done in 4cf5879. The value descriptions now use the dev-site wording.
Reply by Claude Opus 5.5 on Greg's behalf.
| - network | ||
| - other | ||
| description: >- | ||
| The most recent method used to verify the phone number. |
There was a problem hiding this comment.
This description has two gaps compared to dev-site#1775:
- It does not say to omit the field when no verification was attempted. The docs say "Do not include this field if no verification was attempted", and the spec says this for
phone_was_verification_successful. Without the rule, a client can send the catch-allotherfor "not verified". - "The most recent method used to verify the phone number" can read as the method of the most recent successful verification.
phone_was_verification_successfulcan befalse, so the three fields must describe the same most recent attempt. The docs say "The method used for the most recent verification attempt".
Suggestion: "The method used for the most recent verification attempt of the phone number. Omit this field if no verification was attempted."
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Done in 4cf5879, with your suggested text.
Reply by Claude Opus 5.5 on Greg's behalf.
| type: string | ||
| format: date-time | ||
| description: >- | ||
| The date and time of the most recent verification of the phone number, |
There was a problem hiding this comment.
"The most recent verification of the phone number" can read as the most recent successful verification. A client can then send the time of the last successful check with phone_was_verification_successful: false from a later failed attempt, and the fields describe different events. The docs in dev-site#1775 say "The most recent date and time a verification of the ... phone number was attempted, if known".
Suggestion: say "the most recent verification attempt", and say to omit the field when the time is not known, so that a client does not send a placeholder such as the event time.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Done in 4cf5879: "The most recent date and time a verification of the phone number was attempted, if known". Product is still reviewing the dev-site wording, so this can change again.
Reply by Claude Opus 5.5 on Greg's behalf.
| description: >- | ||
| The phone number, without the country code. MaxMind strips punctuation | ||
| characters. After that, the number must contain only digits. | ||
| phone_verification_method: |
There was a problem hiding this comment.
The real target is specs/minfraud.yaml (info.version: 0.1.0) and CHANGELOG.md, which are not in the diff.
This PR adds three request fields but does not change info.version or add a changelog entry. The README says that info.version follows SemVer and tracks changes to the spec. No tag or release exists yet, so this is fine if 0.1.0 is still unreleased. But mm_website already uses a pseudo-version of this module, so two different minFraud documents will both say 0.1.0. Is the plan to bump to 0.2.0 (with a changelog entry) at the first release, or now?
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
We will leave info.version at 0.1.0. Nothing is tagged or released yet, so the first release sets the version and adds the changelog entry.
Reply by Claude Opus 5.5 on Greg's behalf.
| phone_country_code: "1" | ||
| phone_number: 203-000-0000 | ||
| phone_verification_method: delivered_code | ||
| phone_verification_time: "2026-10-01T14:35:00Z" |
There was a problem hiding this comment.
Nit: the shipping example uses 2026-10-01T14:35:00Z, but the shipping request example in dev-site#1775 uses 2026-10-01T14:30:00Z. All other example fields match the docs. Suggest using the same value in both PRs (bundled/minfraud.yaml also has 14:35).
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Done in 30399f3.
Reply by Claude Opus 5.5 on Greg's behalf.
| phone_number: 203-000-0000 | ||
| phone_verification_method: delivered_code | ||
| phone_verification_time: '2026-10-01T14:35:00Z' | ||
| phone_was_verification_successful: true |
There was a problem hiding this comment.
I wonder if we could mark this file as generated in git or something so GitHub doesn't display it as a usual diff.
There was a problem hiding this comment.
Yeah, maybe we can set the git attributes to do that.
There was a problem hiding this comment.
Done in 5f17866: bundled/** linguist-generated=true.
Reply by Claude Opus 5.5 on Greg's behalf.
Match the descriptions on dev.maxmind.com. The method and time describe the most recent verification attempt, not the most recent successful one. The method is omitted when no verification was attempted. The network, delivered_code and other descriptions now give the same examples as the docs, in the same order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
format: date-time follows RFC 3339, which allows a lowercase "t" and "z" and a leap second. The web service rejects these and ignores the input with a warning. Add a pattern to the event time and the phone verification time that allows only an uppercase "T" and "Z", hours 00 to 23, minutes and seconds 00 to 59, and offsets up to 23:59. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub then collapses bundled/ in pull request diffs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds three optional inputs to the minFraud request
billingandshippingobjects:phone_verification_method(delivered_code,network, orother),phone_was_verification_successful(boolean), andphone_verification_time(RFC 3339 date-time). They describe the most recent verification of the phone number. Both objects extend the sharedAddressschema, so the inputs live there. Risk scoring uses the shipping phone when the billing phone is absent. The request example and the bundled spec include the new fields.🤖 Generated with Claude Code
Summary by CodeRabbit